fix: resolve dark mode flicker issue#7533
fix: resolve dark mode flicker issue#7533AnkitRewar11 wants to merge 8 commits intolayer5io:masterfrom
Conversation
Signed-off-by: ankitrewar11 <ankitrewar11@gmail.com>
68be4fd to
a5c4a7d
Compare
|
🚀 Preview for commit a5c4a7d at: https://69bea5593286e0c4d4ee82dc--layer5.netlify.app |
YASHMAHAKAL
left a comment
There was a problem hiding this comment.
This is much better @AnkitRewar11, Good to merge !!
|
@YASHMAHAKAL Thank you so much for the review and support! 🙏 |
|
🚀 Preview for commit 8d09e77 at: https://69befa12ca8054dc493243ab--layer5.netlify.app |
|
@AnkitRewar11 Want to merge this PR? Add it as an agenda item to the meeting minutes, if you would 🙂! Thank you for your contribution! Let's discuss this during the website call tomorrow at 5:30 PM IST | 7 AM CST. |
| @@ -1,22 +1,16 @@ | |||
| //uses isDark state to choose styled-component theme (in themeStyles.js) | |||
| //and use ThemeProvider to allow all styled components access to values via props.theme | |||
|
|
|||
There was a problem hiding this comment.
Are these comments inaccurate or unhelpful?
If they are inaccurate because of the changes that you've made, then update what they say.
If they are not helpful, then update what they say.
| // Apply to DOM immediately | ||
| applyThemeToDOM(themeToApply); | ||
|
|
||
| // Persist to localStorage |
There was a problem hiding this comment.
Why was the existing approach completely ripped out and replaced?
What is precisely wrong with the current approach? Fix that.
If the current approach is completely offbase - ok - explain that and why a complete rewrite is needed.
|
@AnkitRewar11 without details as to where specifically we're failing in the current implementation, and without details as to how your completely new approach resolves the problem, we're flying blind - - and this doesn't work. |
Refactor StyledThemeProvider to ensure consistent theme rendering for SSR. Signed-off-by: Ankit Rewar <171806101+AnkitRewar11@users.noreply.github.com>
Signed-off-by: Ankit Rewar <171806101+AnkitRewar11@users.noreply.github.com>
Refactor theme handling logic in MagicScriptTag component. Update the way themes are injected into the document and change the function signature for onRenderBody. Signed-off-by: Ankit Rewar <171806101+AnkitRewar11@users.noreply.github.com>
Updated comments for clarity and added a fallback for SSR-injected CSS variables during hydration. Signed-off-by: Ankit Rewar <171806101+AnkitRewar11@users.noreply.github.com>
|
🚀 Preview for commit 9ad62cb at: https://69c0e4f81e80d04a59c24468--layer5.netlify.app |
|
🚀 Preview for commit 9736016 at: https://69c0f0bce6282fe4fa13bfa3--layer5.netlify.app |
2048e58 to
42eab57
Compare
|
🚀 Preview for commit 42eab57 at: https://69c0fbc40e47164cd82d99e2--layer5.netlify.app |
42eab57 to
f00fabd
Compare
|
🚀 Preview for commit f00fabd at: https://69c10fd10e471689402d9987--layer5.netlify.app |
… fixes with comments Signed-off-by: Ankit Rewar <AnkitRewar11@users.noreply.github.com>
f00fabd to
f60787a
Compare
|
🚀 Preview for commit f60787a at: https://69c117e633aecf2bd975d0d1--layer5.netlify.app |
|
Hello, you make a fair point. I was wrong to get rid of the original approach and do a full rewrite. That made the diff really hard to look at. It hid the actual fix. That was my mistake. I went back to the version. I put back the architecture, the modern syntax and all the comments. This pull request is now a simple fix. After looking into the server side rendering lifecycle I found out why the master branch was not working and what I needed to change to fix it:
The original code put the script in the body of the html. By the time this runs the browser has already started showing the default light styles. I moved it to the head of the html so that it stops the paint until the css variables are calculated and applied.
The old script put in css variables but it did not set the data-theme attribute on the root of the html. Our global css needs this attribute to work. The variables were there but the background color was not changing before hydration. I added a line of code to fix this.
The server side rendering was calculating the theme. The ThemeManager did not know about it when the client first loaded. It was using the mode by default, which caused a flicker when the page loaded. I made it so that the ThemeManager can pick up the theme that was calculated by the server side rendering.
The master branch was using JSON.parse to parse a string. This is not a way to do it. If any theme string has a quote, in it it breaks and causes a syntax error, which stops the dark mode from working. I changed it so that it passes the string as an object. The diff should be easier to look at now. The flash of unwanted content is completely gone. |
There was a problem hiding this comment.
@AnkitRewar11 i approved your previous changes and you got suggestions to remove wrong comments you had in changes then why 8 more commits ?
were previous changes wrong ?
@leecalcote didn't said your approach was wrong, they want you to explain your approach and the changes you made. you're reacting to every review by 4-5 new commits moving into a circle, try to explain why your approach works, try to defend your changes, try to prove it, and you can defend or back yourself only if you're confident about your approach.
|
@AnkitRewar11 even this time you've responded to review with 5-6 new commits. and the description you post after your commits, it was even relevant to your first commit, and about diff's looking easy, they were much easy before : ) correct me if i'm wrong, Thanks for your efforts 👍 |
You were right my previous approach was also correct. But yesterday Lee suggested a few changes, like adding the comments again, and I’ve done that now. As for the current approach, there isn’t much difference between the two. I just made minimal changes to the old code so that it’s easier to understand. And regarding the new commits, I accidentally made the changes directly here instead of locally, so that’s why so many commits were created. Sorry about that. |
| const initialColorValue = (root.style.getPropertyValue("--initial-color-mode") || "").trim(); | ||
| const actualTheme = window.__theme || initialColorValue || ThemeSetting.LIGHT; | ||
|
|
||
| // Get stored theme from localStorage |
There was a problem hiding this comment.
Please don’t remove the comments.
Description
This PR fixes #7443
This PR resolves the dark mode flicker issue. On page load, the theme
was not being applied before render which caused a visible flicker.
Updated ThemeManager.js, StyledThemeProvider.js, and onRenderBody.js
to properly handle theme initialization and prevent flash of unstyled
content (FOUC).
Notes for Reviewers
Signed commits